binder: Pre-factor out the guts of the BinderClientTransport handshake. #12292
Conversation
# Conflicts: # binder/src/main/java/io/grpc/binder/internal/BinderTransport.java
…-java into isolated-process-2
# Conflicts: # binder/src/main/java/io/grpc/binder/internal/BinderTransport.java
# Conflicts: # binder/src/main/java/io/grpc/binder/internal/BinderTransport.java
# Conflicts: # binder/src/main/java/io/grpc/binder/internal/BinderTransport.java
# Conflicts: # binder/src/main/java/io/grpc/binder/internal/BinderTransport.java
# Conflicts: # binder/src/main/java/io/grpc/binder/internal/BinderTransport.java
# Conflicts: # binder/src/main/java/io/grpc/binder/internal/BinderTransport.java
# Conflicts: # binder/src/main/java/io/grpc/binder/internal/BinderTransport.java
…-java into isolated-process-2
… into isolated-process-2
|
@mateusazis PTAL |
| if (readyTimeoutFuture != null) { | ||
| readyTimeoutFuture.cancel(false); | ||
| readyTimeoutFuture = null; | ||
| class LegacyClientHandshake extends ClientHandshake { |
| int remoteUid = Binder.getCallingUid(); | ||
| restrictIncomingBinderToCallsFrom(remoteUid); | ||
| attributes = setSecurityAttrs(attributes, remoteUid); | ||
| authResultFuture = checkServerAuthorizationAsync(remoteUid); |
There was a problem hiding this comment.
Not sure about the plans for the next PR, but it seems like authResultFuture is never used by the parent class anymore except during cancellation.
It feels a bit weird to see one class setting another one's fields, even if they are nested. Could we move authResultFuture into this one and add a cancel method to ClientHandshake?
Ditto possibly for attributes; couldn't fully track all the places where it is used.
| * | ||
| * <p>Supports a clean migration away from the legacy approach, one client at a time. | ||
| */ | ||
| abstract class ClientHandshake { |
There was a problem hiding this comment.
Could we design at as an interface instead?
There was a problem hiding this comment.
No because I want its methods to be @GuardedBy("BinderClientTransport.this") which isn't well defined for an interface or anything but an inner class.
There was a problem hiding this comment.
Could we apply the annotation to the BinderClientTransport.handshake field instead and then use an interface?
Either with an interface or an abstract class, it sounds weird to guard a method based on another class's fields (despite the outer-inner class relationship). Feels like we have a purely abstract API making very strong assumptions about its call site.
| * | ||
| * <p>Supports a clean migration away from the legacy approach, one client at a time. | ||
| */ | ||
| abstract class ClientHandshake { |
| int remoteUid = Binder.getCallingUid(); | ||
| restrictIncomingBinderToCallsFrom(remoteUid); | ||
| attributes = setSecurityAttrs(attributes, remoteUid); | ||
| authResultFuture = checkServerAuthorizationAsync(remoteUid); |
There was a problem hiding this comment.
It would be great if authResultFuture could be a local variable, so it gets garbage-collected once the auth is over and we don't need it anymore. I see we need to keep it around to support cancellation though.
Could we clear it after it completes?
Another pattern I have seen is to keep a collection of "cancellable stuff (or futures)" and just remove items when they are no longer subject to cancellation.
There was a problem hiding this comment.
Yes, authResultFuture holds the result of evaluating a long-blocking or async SecurityPolicy so we want to cancel it if this transport instance terminates before it completes. All implementations of ClientHandshake will evaluate the SecurityPolicy (just at different times) and all will want this same cancellation behavior so I'd like to leave the future and cancellation code as it is.
We could certainly clear this future after completion or put it in a collection but the current code doesn't do this and my goal with this PR is no behavior changes in a way that's obvious by inspection. LMK if you feel strongly.
There was a problem hiding this comment.
Sounds good. Agreed that this is an optimization that we do not need to do right now.
Part 1 of fixing #12293. No behavior change intended this is a pure refactor.